Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

extend and fix data url imports #534

Conversation

romainmenke
Copy link
Collaborator

@romainmenke romainmenke commented Jul 20, 2023

  • add support for url encoded data urls
  • fix handling of embedded relative imports

@romainmenke romainmenke requested a review from RyanZim July 20, 2023 12:20
@@ -1,24 +1,6 @@
p { color: green; }
@import url(foo.css);p { color: green; }p { color: blue; }@media (min-width: 320px){@layer foo{
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If a data url eventually unfurls into a regular import with a relative path it should stop and must not inline the relative import.

Each import is resolved relatively to it's parent and when the parent is a data url there isn't any domain or base url to resolve a url against.

I think usage of data urls was already very niche and that it is extremely unlikely that anyone depends on the old behavior.

Comment on lines 97 to 99
if (dataURL.isValid(stmt.from)) {
return
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps leave a comment on what we're doing here, and why? Won't be obvious to our future selves.

@romainmenke
Copy link
Collaborator Author

Thank you for the review 🙇

@romainmenke romainmenke merged commit 81a7701 into master Jul 22, 2023
3 checks passed
@romainmenke romainmenke deleted the extend-and-fix-data-url-imports--adventurous-uakari-79de749a0a branch July 22, 2023 21:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants